Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicitly set responder on partial mocks #532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

floehopper
Copy link
Member

@floehopper floehopper commented Aug 15, 2022

This relates to #149 and #531.

All the tests seem to pass, but it would be worth doing a bit more
thinking about the change in Mock#check_responder_responds_to where we
now set include_all to true for the call to Object#respond_to?.

Also it's only really worth doing this if the investigation in #531
means that it's definitely worthwhile. It would probably also be worth
doing some spiking on the responder-related solution proposed in #149.

@floehopper
Copy link
Member Author

Note that the build failure on Ruby v1.8 looks like it's due to blocks not supporting argument default values. That was only introduced in Ruby v1.9.

@floehopper floehopper force-pushed the implicit-responder-for-partial-mocks branch from 3be2e8b to 4e30bc6 Compare August 15, 2022 17:43
@floehopper
Copy link
Member Author

Note that the build failure on Ruby v1.8 looks like it's due to blocks not supporting argument default values. That was only introduced in Ruby v1.9.

Now fixed via #533.

@floehopper floehopper force-pushed the implicit-responder-for-partial-mocks branch from 4e30bc6 to 446f107 Compare August 20, 2022 18:59
@floehopper
Copy link
Member Author

Rebased now that #533 has been merged.

@floehopper
Copy link
Member Author

See also #580 & #583.

@floehopper
Copy link
Member Author

I'm not 100% sure about the changes and the commit note and PR description could be improved, but I think this is now at the point where it's ready for a proper review.

  • Explain the changes in RespondsLikeTest in the commit note
  • Update the PR description

We now automatically set a responder on mock object which are used for
partial mocks.

Having made the change above, I had to set include_all to true for the
call to Object#respond_to? in Mock#check_responder_responds_to in order
to fix a load of broken tests.

The legacy_behaviour_for_array_flatten condition in
Mock#check_responder_responds_to is needed to avoid a regression of #580
in Ruby < v2.3.

Hopefully this is a small step towards having
Configuration.prevent(:stubbing_non_existent_method) check Method#arity
and/or Method#parameters (#149) and rationalising
Configuration.stubbing_non_existent_method= & Mock#responds_like (#531).
@floehopper floehopper force-pushed the implicit-responder-for-partial-mocks branch from 6648213 to 42da834 Compare December 31, 2022 15:08
floehopper added a commit that referenced this pull request Dec 31, 2022
When a method doesn't accept keyword arguments.

In this scenario keyword or Hash-type arguments are assigned as a single
Hash to the last argument without any warnings and strict keyword
matching should not have any effect.

This is an exploratory spike on fixing #593.

* This has highlighted a significant problem with partial mocks in #532.
  The method obtained from the responder is the stub method defined by
  Mocha and not the original. This effectively makes it useless!

* I'm not sure the method_accepts_keyword_arguments? belongs on
  Invocation, but that's the most convenient place for now. It feels as
  if we need to have a bit of a sort out of where various things live
  and perhaps introduce some new classes to make things clearer.

* We might want to think ahead a bit at what we want to do in #149 to
  decide the best way to go about this.

* I'm not sure it's sensible to re-use the Equals matcher; we could
  instead parameterize PositionalOrKeywordHash, although the logic in
  there is already quite complex. Conversely if this is a good approach,
  it might make more sense to do something similar when creating a hash
  matcher for a non-last parameter to further simplify the code.

* I haven't yet introduced any acceptance tests for this and I suspect
  there might be some edge cases yet to come out of the woodwork. In
  particular, I think it's worth exhaustively working through the
  various references mentioned in this comment [1].

[1]: #593 (comment)
@floehopper
Copy link
Member Author

Some work on #605 has highlighted a potentially serious flaw in this approach. Currently for partial mocks, the method obtainable from the responder is the stub method defined by Mocha and not the original method which is what we want. More thought required!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant